Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for PHPStan 2 #348

Merged
merged 23 commits into from
Nov 21, 2024
Merged

Conversation

sasezaki
Copy link
Contributor

@sasezaki sasezaki commented Nov 15, 2024

on top of #345, #347

and do " Remove not longer required stubs" @see #309 .


Todos - needs HELP!

  • Update README example code.
  • Update .docker (or remove?)
  • Update gha matrix to test with PHP 8, PHPUnit 10,11

This PR includes a lot of changes to pass CI.

requiments, dependency

  • Update requirements PHPUnit >=9.1 .
    • Becaouse, run test with PHPUnit 8 would cause deprecation warning.
    • PHPUnit 9 requires PHP 7.3, and PHPStan requires PHP 7.4. So update requirements is acceptable, I think.
  • Add requiments phpspec/prophecy-phpunit
  • Update php-cs-fixer 2 to 3. ( .php_cs to .php-cs-fixer.php)
    • to run under recent PHP 8 versions
    • I'm not familiar with php-cs-fixer, so if you find any problem, please tell me.

tests

@sasezaki sasezaki changed the title [PHPStan 2.0] [WIP][PHPStan 2.0] Nov 15, 2024
@sasezaki sasezaki marked this pull request as ready for review November 15, 2024 14:56
@sasezaki sasezaki requested a review from Jan0707 as a code owner November 15, 2024 14:56
@sasezaki sasezaki marked this pull request as draft November 15, 2024 14:57
@sasezaki sasezaki marked this pull request as ready for review November 16, 2024 12:54
@sasezaki sasezaki changed the title [WIP][PHPStan 2.0] Add support for PHPStan 2 Nov 16, 2024
@sasezaki sasezaki marked this pull request as draft November 16, 2024 13:03
@sasezaki sasezaki marked this pull request as ready for review November 16, 2024 13:24
src/Type/Prophet/ProphesizeDynamicReturnTypeExtension.php Outdated Show resolved Hide resolved
src/PhpDoc/ObjectProphecy/TypeNodeResolverExtension.php Outdated Show resolved Hide resolved
@@ -92,9 +100,9 @@ public function getTypeFromMethodCall(
[
Type\TypeCombinator::intersect(
new Type\ObjectType($className),
...$calledOnType->getTypes()
//...$calledOnType->templ
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commenting this looks wrong.

I would also suggest trying to remove that extension entirely (without removing the tests) as I think the phpdoc tags in ObjectProphecy should be enough to describe that already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is my fault, it's pending in #347 since I didn't know how to do this.

Copy link
Contributor Author

@sasezaki sasezaki Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof

I think the phpdoc tags in ObjectProphecy should be enough to describe that already.

I agree with your suggestion.

sadly but, current prophecy's phpdoc (from phpspec/prophecy#574) is only about this-out.

     * @template U of object
     * @phpstan-param class-string<U> $interface
     * @phpstan-this-out static<T&U>
     */
    public function willImplement($interface)

not defined like @phpstan-return static<T&U>.

so we got bellow errors when remove this WillExtendOrImplementDynamicReturnTypeExtension removed.

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   test/StaticAnalysis/Test/ObjectProphecy/WillImplementTest.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  62     Parameter #1 $bar of method JanGregor\Prophecy\Test\StaticAnalysis\Src\BaseModel::bar() expects JanGregor\Prophecy\Test\StaticAnalysis\Src\Bar, JanGregor\Prophecy\Test\StaticAnalysis\Src\Foo given.
         🪪  argument.type
  84     Method JanGregor\Prophecy\Test\StaticAnalysis\Test\ObjectProphecy\WillImplementTest::createProphecy() should return Prophecy\Prophecy\ObjectProphecy<JanGregor\Prophecy\Test\StaticAnalysis\Src\Bar> but returns
         Prophecy\Prophecy\ObjectProphecy<JanGregor\Prophecy\Test\StaticAnalysis\Src\Foo>.
         🪪  return.type
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  • refs. ( debugging with dumpType)
diff --git a/test/StaticAnalysis/Test/ObjectProphecy/WillImplementTest.php b/test/StaticAnalysis/Test/ObjectProphecy/WillImplementTest.php
index 05b61de..62693c7 100644
--- a/test/StaticAnalysis/Test/ObjectProphecy/WillImplementTest.php
+++ b/test/StaticAnalysis/Test/ObjectProphecy/WillImplementTest.php
@@ -50,7 +50,12 @@ final class WillImplementTest extends Framework\TestCase

     public function testCreateProphecyInTestMethod(): void
     {
+        $prophecyParamOut = $this->prophesize(Src\Foo::class);
+        $prophecyParamOut->willImplement(Src\Bar::class);
+        \PHPStan\dumpType($prophecyParamOut);
+
         $prophecy = $this->prophesize(Src\Foo::class)->willImplement(Src\Bar::class);
+        \PHPStan\dumpType($prophecy);
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   test/StaticAnalysis/Test/ObjectProphecy/WillImplementTest.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  55     Dumped type: Prophecy\Prophecy\ObjectProphecy<JanGregor\Prophecy\Test\StaticAnalysis\Src\Bar&JanGregor\Prophecy\Test\StaticAnalysis\Src\Foo>
  58     Dumped type: Prophecy\Prophecy\ObjectProphecy<JanGregor\Prophecy\Test\StaticAnalysis\Src\Foo>
  67     Parameter #1 $bar of method JanGregor\Prophecy\Test\StaticAnalysis\Src\BaseModel::bar() expects JanGregor\Prophecy\Test\StaticAnalysis\Src\Bar, JanGregor\Prophecy\Test\StaticAnalysis\Src\Foo given.
         🪪  argument.type
  89     Method JanGregor\Prophecy\Test\StaticAnalysis\Test\ObjectProphecy\WillImplementTest::createProphecy() should return Prophecy\Prophecy\ObjectProphecy<JanGregor\Prophecy\Test\StaticAnalysis\Src\Bar> but returns
         Prophecy\Prophecy\ObjectProphecy<JanGregor\Prophecy\Test\StaticAnalysis\Src\Foo>.
         🪪  return.type
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a bug in phpstan: phpstan/phpstan#8439

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, that can be solved with a conditional type: https://phpstan.org/r/4bd81d3d-c0cf-40bf-a8a8-9b951e317771

 * @return ($class is class-string ? ObjectProphecy<T> : ObjectProphecy<object>)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional type does not solve the phpstan bug with willExtend. It only hides it in the case of prophesize. See my new comment on the bug.

Copy link
Contributor Author

@sasezaki sasezaki Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof

This is actually a bug in phpstan: phpstan/phpstan#8439

oh, my bad! I had did not understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at 7061ef1

I rolled back the comment-out.
I assume the reason for the code being commented out was that a method-not-found error would be reported by phpstan if the GenericObjectType was not properly asserted. @Jean85

@Jean85
Copy link
Contributor

Jean85 commented Nov 19, 2024

phpspec/prophecy-phpunit 2.3.0 is now released: https://github.com/phpspec/prophecy-phpunit/releases/tag/v2.3.0
Note that this implicitly bumps the Prophecy requirement to 1.18: https://github.com/phpspec/prophecy-phpunit/blob/8819516c1b489ecee4c60db5f5432fac1ea8ac6f/composer.json#L16

The only remaining blocker seems phpstan/phpstan#8439, but we can keep the WillExtendOrImplementDynamicReturnTypeExtension as a workaround for now. WDYT?

@@ -87,14 +95,16 @@ public function getTypeFromMethodCall(
$className = $scope->getClassReflection()->getName();
}

\assert(\get_class($calledOnType) === Type\Generic\GenericObjectType::class);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid assert($calledOnType instanceof Type\Generic\GenericObjectType::class);
( see https://phpstan.org/blog/why-is-instanceof-type-wrong-and-getting-deprecated )
.. I'm not sure this assertion check is good, or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking get_class() === is actually a lot worse than instanceof, as it would even break for child classes. Such check is actually breaking the LSP rule of SOLID if the class being checked is not final (and is strictly equivalent to instanceof for final classes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's absolutely correct. I think I've been relying too much on workarounds.

Looking at PHPStan's own baseline, I noticed that a lot of phpstanApi.instanceofType remains. I think I'll revert it back to instanceof.
https://github.com/phpstan/phpstan-src/blob/2.0.2/phpstan-baseline.neon

@sasezaki
Copy link
Contributor Author

@Jean85

The only remaining blocker seems phpstan/phpstan#8439, but we can keep the WillExtendOrImplementDynamicReturnTypeExtension as a workaround for now. WDYT?

I think keeping the WillExtendOrImplementDynamicReturnTypeExtension is still needed.

@sasezaki sasezaki mentioned this pull request Nov 21, 2024
@alexander-schranz
Copy link
Collaborator

I would keep it aslong as phpstan/phpstan#8439 is not solved. If that is solved we can in future versions remove it and increase the phpstan min version.

@Jean85
Copy link
Contributor

Jean85 commented Nov 21, 2024

Ok, so we're ok with leaving the WillExtendOrImplementDynamicReturnTypeExtension. Is this PR mergeable then? @sasezaki asked for help on updating the GHA matrix, I can do that but I need to know where; if this isn't merged, I don't know if I should start another PR or not.

@alexander-schranz alexander-schranz merged commit cc8c094 into Jan0707:master Nov 21, 2024
5 checks passed
@alexander-schranz
Copy link
Collaborator

@Jean85 fine to continue things in another Pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants